Coverage decreased (-0.21%) to 96.61% when pulling 22751c2 on performance-comparison into cd9191a on master.

@jdantonio How many time did you run the test?

@lucasallan Only a couple of times. It wasn't a comprehensive test. It's just something I knocked out this morning before starting work.

I knew we would have a small performance impact - pure Java is faster than JRuby.

With that being said, I think we should stick with the "JRuby interop" version because it makes the gem easier to maintain and the performance hit is not something that would make someone not use the gem.

I was actually surprised by the difference in performance, but that may simply be my lack of understanding of JRuby internals. In both cases we're wrapping a pure-Java java.lang.ref.* class. In both cases we're putting a Ruby object inside the pure-Java ref class. In both cases we have to cross the barrier between the Ruby code and the Java code. Yet one is 4-5 times faster than the other. What's going on in the JRuby-interop version that makes it so much slower?

In this case I think the performance difference may outweigh the maintenance difference. The Java code is already written as are the build tasks. Since it's just a wrapper there should be no need to change the Java code unless Oracle adds new methods to the java.lang.ref.* classes. We can reasonably expect that code to be very stable. But I think we can also reasonably expect applications that use this gem to be performance-sensitive.

I'm curious to hear what others on the team think.

Sounds good. I'll revert it and we should also remove it from the Roadmap.

We don't have to revert yet. There is no rush. We can give the others a day or two to respond. Our teammates are mostly in Europe so there is a significant time difference.

Oh yes, sorry! I forgot to mention that I'll wait the team.

We need to let the benchmark warm up, running it for much longer and with the --server flag getting all the optimizations.

Sorry for useless comment, I made it in a hurry. You've already did what I've suggested. Even though I've tried with more cycles 50_000_000 and with WARM_UPS = NUM because server JIT threshold is around 10k. Updated results are pushed.

I think it makes sense to revert.

Coverage remained the same at 96.83% when pulling 392056f on performance-comparison into cd9191a on master.

I agree with reverting.

This also has me thinking about the wrapped objects in concurrent-ruby, such as AtomicBoolean and AtomicFixnum. I think we should experiment with pure-Java wrappers for those objects (following this pattern) and see if it makes a difference. If there is a similar performance difference we may want to adopt this pattern in that gem.

@jdantonio yeah, good point.

I'll revert it tonight.